Conversation
|
@tagyoureit Could you take a look when you get a chance? I appreciate it. Thank you! |
tagyoureit
left a comment
There was a problem hiding this comment.
Review: Healthcheck Dynamic Port
Nice improvement — the script is clean and well-structured. One functional concern and a couple of minor suggestions:
🟡 Medium: Environment variable override not detected
The config loader in server/config/Config.ts applies POOL_WEB_SERVERS_HTTP_PORT (and similar env vars) at runtime in-memory — they're never written back to config.json on disk. If a user configures their port solely via environment variable (as documented in docker-compose.yml comments), the healthcheck will still read 5150 from the static file and fail.
Suggested fix in docker/healthcheck.js — after reading the config file, add an env var fallback:
// Match the config loader's env var behavior
const envPort = process.env.POOL_WEB_SERVERS_HTTP_PORT;
if (envPort) {
const parsed = parseInt(envPort, 10);
if (!isNaN(parsed)) port = parsed;
}💡 Minor: curl is now unused
The runtime stage still installs curl (apk add --no-cache curl) which was only needed for the old healthcheck. Removing it saves ~2MB from the image.
💡 Minor: COPY placement
The COPY docker/healthcheck.js /usr/local/bin/healthcheck.js line is placed after USER node. While this works (COPY runs as root at build time regardless), it's more conventional to group it with the other COPY statements above the USER directive.
Otherwise the implementation looks solid — proper timeout handling, clean status code check, graceful config fallback with optional chaining.
Hello!
My setup is a bit different where I'm not using a bridge network but a macvlan network, so I can't just expose 5150 to a different port. Instead, I've modified my config.json to have:
This PR modifies the Dockerfile and the compose file so that it calls docker/healthcheck.js instead, where it gets the port from /app/config.json, and falls back to 5150 as the default
Before and after:
The top container is using macvlan and the bottom one is using bridge; both are using the same config